-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
handling terminals created by vscode-Python #1511
handling terminals created by vscode-Python #1511
Conversation
Hi @tomasnobrega, thank you for submitting this PR and very nice work tracking down the cause of the error! Looking at the screenshot, it appears that the Python extension may have recently started to use the I'm going to leave a couple of comments on the code. After those are resolved, I'll do some manual testing. (At that point I might also need to reach out to one of the more active maintainers for help on resolving the failed CI checks.) |
Hi Andy thank you for the helpful comments, I will make the changes over the weekend |
@andycraig I just commited with requested changes. In my VS Code its working. Let me know if you need anything else |
@tomasnobrega Thank you very much for making these changes. I think it's fine to merge now now. I have written some notes below about some manual testing that I did, which turned up an edge case, but I think we can ignore it at present. Details of manual tests I ranWith a file Setting Situation: There are 3 R terminals. The current terminal is the 2nd R terminal.
✅ PASSED Situation: There are multiple R terminals. The current terminal is not an R terminal.
✅ PASSED Situation: Works when there is only one terminal, it is an R terminal, and it has just been created. (This did not work at one point due to a bug in VS Code.)
✅ PASSED Situation: There WAS a Python terminal, but it has been closed. There is an R terminal. (This did not work at one point, probably due to a bug in VS Code or the Python extension.)
✅ PASSED Setting Situation: There are no terminals.
✅ PASSED Situation: There is an open terminal. Code is run in that terminal.
✅ PASSED Situation: There WAS a Python terminal, but it has been closed. There are no open terminals. (This did not work at one point, probably due to a bug in VS Code or the Python extension.)
❌ FAILED (no VS Code message was shown) The tests all passed except the last one, for the situation where In future it would be nice to handle that edge case too, but this PR solves a problem that many users are experiencing and I am happy for it to be merged in its current form. |
@renkun-ken @ElianHugh @ManuelHentschel |
@andycraig I'm fairly certain the stops are just issues with the runner, it started happening recently and tends to resolve (at least for me) if the tests are reran until they succeed. Probably worth investigating, but I don't believe it has anything to do with this PR Edit: looks like the runner is deprecated: https://github.com/GabrielBB/xvfb-action and the macos error mirrors the one linked here: microsoft/vscode#200708 |
Perhaps a node.js version issue? |
LGTM, thank you! @eitsupi I'll create a new issue to track this, I'm not 100% familiar with the github actions used in this repo |
What problem did you solve?
If Python extension and R extension are open at the same time on VSCode a conflict emerges because Python extension creates a new terminal.
vscode-R currently checks for only one terminal available at (vscode.window.terminals.length === 1). I propose to fix that by actively excluding the "Deactive" terminal.
This issue has been reported before:
#1459 (comment)
Note all 3 screenshots the second terminal is "Python ... Deactive" indicating the same root cause.
(If you do not have screenshot) How can I check this pull request?
To recreate the bug you should install Python VSCode extension and run some Python on VSCode (anything, just to activate the extension). Then close all R terminals and attempt to run a new R line of R code by using "Ctl + Enter" or "Cmd + Enter", should run into the error identifying terminal.